I wasn't sure if we were going to move these to core. These modules should not be useful on a typical server, so they seem to me like they could be a in higher-level crate. But, as I've said many times, I'm still very confused about what the core crate is.

I wasn't sure if we were going to move these to core.

They should move to core because we cannot move the actual relevant stuff which are the traits over. I also have commits doing this.

These modules should not be useful on a typical server

I think I've tried to make it clear that this isn't about the server use-case (#6284 (comment)). Its about enabling non-server, non-CLI use-cases which just want the traits like Mahou or another creative application of them.

But, as I've said many times, I'm still very confused about what the core crate is.

It also doesn't seem that any further explanation from my side is going to help with this. 🙁

I think the first 2 commits are uncontroversial, in case you want to start by landing just those.

They should move to core because we cannot move the actual relevant stuff which are the traits over. I also have commits doing this.

I agree about ref_name, but I'm not sure about revset_parser and fileset_parser.

I think I've tried to make it clear that this isn't about the server use-case (#6284 (comment)). Its about enabling non-server, non-CLI use-cases which just want the traits like Mahou or another creative application of them.

I understand that the server use case is not your priority, but it's the one I'm most familiar with, and I know at least a few companies are interested in that use case. So perhaps a possibility is to have a jj-core crate that has more or less only what a forge server would need. Then we can have jj-local crate or something with things needed for things like jj-cli and gg. I'm not saying it has to be that way, it's just the only reasonably clear end state I can imagine.

They should move to core because we cannot move the actual relevant stuff which are the traits over. I also have commits doing this.

I agree about ref_name, but I'm not sure about revset_parser and fileset_parser.

I kind of agree. revset_parser was moved because impl Display for RefSymbol depends on the parser. Alternative options are:

1. Remove impl Display and let UI functions format symbols explicitly. (tedious?)
2. Add newtype for UI stuff. (probably not useful)
3. Duplicate minimal functions to test and escape symbols as needed.

FWIW, depending on your definition of "core", it might be better to move file_util back to jj-lib or into a middle crate.

• Remove impl Display and let UI functions format symbols explicitly. (tedious?)

I had Claude implement try this out. Here's what it looks like: bc7b049. Claude pointed out that we still have dependencies on format_symbol() for error messages in working_copy.rs and view.rs, so we'd have to do something about those later (since we want to move them to core). Since it's just about error messages, it's less important. We can include the literal values in the structured error variant so callers can format them accurately if necessary.

FWIW, depending on your definition of "core", it might be better to move file_util back to jj-lib or into a middle crate.

Yes, good point. I think we should do that now that you mention it. But let's wait a little and see if we get any other opinions on where the line should be drawn.

• Remove impl Display and let UI functions format symbols explicitly. (tedious?)

I had Claude implement try this out. Here's what it looks like: bc7b049. Claude pointed out that we still have dependencies on format_symbol() for error messages in working_copy.rs and view.rs, so we'd have to do something about those later (since we want to move them to core).

Yeah, I rejected the idea for this reason. It seems better, at least, to have minimal formatting (and paired parsing) functions in the core:

• dsl_util::escape_string()
• dsl_util::StringLiteralParser (because the implementation must be compatible with escape_string())
• Stripped-down version of revset_parser::is_identifier()

It would be flaky if we had to be careful not to use the default ToString impl for errors containing ref symbols.

#10069

Removed patches to move parsers into core. Added a few more doc comments.